-
Notifications
You must be signed in to change notification settings - Fork 2.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
node: fix the rest of test-process #16026
base: main
Are you sure you want to change the base?
Conversation
Updated 9:12 PM PT - Jan 3rd, 2025
❌ @nektro, your commit 83ce207 has 2 failures in
🧪 try this PR locally: bunx bun-pr 16026 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments, let me know when ready for another pass
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't make error:
print as Error:
- There are very likely 3rd-party scripts depending on this behavior
- It's aesthetically worse
We can make whatever tests that specifically check for the string "Error:" instead check for "error:", or we can make sure it's using util.inspect isntead of Bun.inspect / console.log in the appropriate places.
supercedes #14621
fixes all the tests that don't depend on missing behavior in
node:child_process
ornode:v8
a few missing for
process.env
that will get a separate followupnot done, pushing now so the robobun comment is at the top